Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request.

@dotnet-policy-service agree company="Imatra"

Looks great!

I'll let @MikeAlhayek merge it and backport it to make sure it's included in the next patch/minor release.

Happy holidays! 🎄

Thank you! Happy holidays

@kevinchalet I see couple of issues with the code. I think the code should look like this instead. do you agree?

I think the old code should be kept. Instead, replace the line

settings.Authority = !string.IsNullOrEmpty(model.Authority) ? new Uri(model.Authority, UriKind.Absolute) : null;

with this code

        if (string.IsNullOrEmpty(model.Authority))
        {
            context.Updater.ModelState.AddModelError(Prefix, nameof(model.Authority), S["A tenant or authority value is required."]);
        }
        else 
        {
            settings.Authority = new Uri(model.Authority, UriKind.Absolute)
        }

If Authority should be only validated if there is not Tenant, then we may need to fix the error to this instead

context.Updater.ModelState.AddModelError(Prefix, nameof(model.Authority), S["A tenant or authority value is required."]);

I think the code should look like this instead. do you agree?

Personally, I'd even remove all this validation code and simply rely on OpenIdValidationService, that eventually performs similar checks anyway:

@kevinchalet The OpenIdValidationService is good. But in the driver, we assign prefix for the validation error key names. Also, if for some reason the property names in OpenIdValidationSettingsViewModel did not match the same name, some errors may not be printed on the UI.

I think its better to leave the validation in the driver "I know it repeated logic", but it's clear what we are validating and what keys we are settings with the errors.

Please check out the updated validation logic. If you are good with this, I'll merge and portback.

Also, if for some reason the property names in OpenIdValidationSettingsViewModel did not match the same name, some errors may not be printed on the UI.

Well, if you're concerned by that, we should be duplicating all the validation checks and not just a single one? 😄

If you strongly think we should have a duplicated check, let's merge it. Otherwise, just remove it.

I'll merge this one and back port it. We can submit another PR in v3 that removed the validation from the driver.

Congratulations on your first PR merge! 🎉 Thank you for your contribution! We're looking forward to welcoming other contributions of yours in the future. @all-contributors please add @salmattia for code.

@github-actions[bot]

I've put up a pull request to add @salmattia! 🎉

/backport to release/2.1

Started backporting to release/2.1: https://github.com/OrchardCMS/OrchardCore/actions/runs/12486787988

Hi @salmattia, @kevinchalet, @MikeAlhayek, I am still experiencing this issue in Orchard Core 2.1.3. The Authorization Server tenant is mandatory, making it impossible to save an external authority.

Are you encountering the same problem in this version? How can I apply the fix?

Thank you

P.S.:

Now I fixed with a workaround, modifying the value of OpenIdValidationSettings in the DB, in the Document table, row Type OrchardCore.Settings.SiteSettings, OrchardCore.Settings, from this:

"OpenIdValidationSettings":{"DisableTokenTypeValidation":false,"Tenant":"Default"}}

to this:

"OpenIdValidationSettings":{"Audience":"Store.SSO","Authority":"http://host.docker.internal:12150/","DisableTokenTypeValidation":false,"Tenant":null,"MetadataAddress":null}}

We did not release a new version of OC that includes this fix. This will be part of OC 3.0 and 2.1.4 if we decided we will release a new version.

This will be part of OC 3.0 and 2.1.4 if we decided we will release a new version.

Since at least 2 people are affected by this bug, we should definitely backport it. Bumping the OpenIddict dependency to 6.0.0 as part of a new minor release would also be nice (otherwise folks will stay in an unsupported state until OC 3.0 ships).

It's already backported just need a release.

Why bump up OpenIdDict library at the same time, I don't want that to cause unexpected issue for others since that is a minor

It's already backported just need a release.

By backport it, I mean create a branch that includes this change (already done) AND release a new version with the fix. Having a branch and no release is totally useless for those impacted by this bug 😄

Why bump up OpenIdDict library at the same time,

As I said, the 5.x version is no longer supported. 3.0 has no date assigned, so it could ship in a few months... a period during which users can't migrate their OC app to use OpenIddict 6.0, which is the supported version.

Yes I understand. It's back ported just in case we want to release another patch. I can work on a new patch next week if I have time.

However, I am not sure upgrading OpenIdDict in a patch release is a good idea as it could break people. @sebastienros thought on upgrading OpenIdDict to 6.0 in 2.1.4?